fix(cli): avoid dangling symlink temp path - #1467
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Thanks, that’s a fair point. This PR is scoped to the static dangling-symlink case described in the report: a pre-existing zx.mjs dangling symlink being treated as an available temp path because of existsSync() semantics. I agree this does not fully eliminate the TOCTOU class between path selection and writeFile(). A stronger follow-up would be to make the creation itself atomic, for example with exclusive create semantics and no-follow behavior where supported, or by moving this flow to a securely-created temp directory. I kept this patch narrow to avoid changing the temp-file lifecycle more broadly in the security fix, but I’m happy to either update the Security Impact wording to make the scope explicit, or send a follow-up hardening PR for atomic temp-file creation if maintainers prefer. So the intended coverage here is the static dangling-symlink footgun, not a full race-resistant temp-file primitive. |
1 participant
Title
Fix dangling symlink handling for CLI temporary script paths
Description
This PR fixes a symlink-handling issue in the CLI temporary script file path selection used by
--eval, stdin, and remote script execution.Previously,
getFilepath()usedfs.existsSync()to determine whether a candidate temp path such aszx.mjswas available.existsSync()returnsfalsefor dangling symlinks, which meant a pre-existing dangling symlink could be treated as a free path. The subsequentfs.writeFile()call would then follow the symlink and create the symlink target.This change replaces that availability check with an
lstatSync()-based helper, so symlinks, including dangling symlinks, are treated as occupied paths and skipped.Changes
lstatSync()instead ofexistsSync()when checking CLI temp path availability.build/cli.cjsoutput.--evalwith a danglingzx.mjssymlink.Security Impact
Prevents attacker-controlled dangling symlinks in the working directory from being selected as temporary script paths and written through during CLI execution.
Testing
npx prettier --check src/cli.ts test/cli.test.jsnpx tsc --project tsconfig.json --noEmit